-
-
Notifications
You must be signed in to change notification settings - Fork 269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow hyphenated ranges in compatability specs #1410
Allow hyphenated ranges in compatability specs #1410
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a "feature" that we might want to think about backporting to 1.x
Codecov Report
@@ Coverage Diff @@
## master #1410 +/- ##
==========================================
+ Coverage 87.27% 87.31% +0.03%
==========================================
Files 25 25
Lines 5383 5398 +15
==========================================
+ Hits 4698 4713 +15
Misses 685 685
Continue to review full report at Codecov.
|
I wouldn't oppose that, but I am curious why you think we should backport this. |
Because otherwise packages that want to support this have to drop the LTS version of julia. Maybe it isn't a big deal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some doc comments.
docs/src/compatibility.md
Outdated
|
||
```toml | ||
[compat] | ||
PkgA = "0.1 - 0.2.3" # [0.1, 0.2.3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given https://discourse.julialang.org/t/how-can-we-encourage-julia-package-developers-to-release-version-1-0-0/29124, maybe one way is to use post 1.0 numbers in examples like this? 😄 At least in addition, to the current example.
I like this. To fully obsolete #1349, this would also need to document |
A concern that should be considered is if we want to support pre-release versions then |
We could just disambiguate that as a version range, which seems like the more common case, although then there isn’t any way to write the pre-release version, whereas if we interpreted this as the pre-release version the version range can still be written as |
Maybe enforce having a space on the sides of the hypen for now? |
Would it be nuts to use a different character than |
|
At that point I'd rather have |
|
Another option may be |
I think with the current push to encourage dependency version limits (upper bounds now required for automatic merging on General), this feature would be very very helpful, and I'd be very glad if it were backported to v1.0! I just ran into a situation where this package resolution failed, because I upgraded "ElasticArrays" to v1.0 and updated another package to require it - but another dependency that I wanted required "ElasticArrays" v0.2. I can work around that by just setting "ElasticArray <= 2.0" on my other package - but what if I need a lower bound as well? I have a distinct feeling that without the ability to specify deps version ranges, will run into (extremely frustrating) situations in which package resolution fails more and more often, as package authors put tighter bounds on dependency versions. |
As a general rule, features aren't backported. Pkg is a bit special though, and I've been toying around with the idea of making releases of 1.0 with an updated version of Pkg. |
@fredrikekre reminded me that it's already possible to specify a comma-separated list of versions -somehow I missed that or forgot about it, sorry! So since there's a decent workaround, I'll have take back my earlier statement about this needing a backport to 1.0. |
I completely forgot about this pull request. What was the latest decision? Enforce a space on either side of the hyphen? I.e. |
Alright, I have updated the PR with all of the review comments. I am enforcing the rule that there must be whitespace on both sides of the hyphen. That way, there is no confusion/ambiguity with prerelease notation. I think this is ready to merge. cc: @StefanKarpinski @KristofferC @fredrikekre |
Bump @StefanKarpinski @KristofferC @fredrikekre. Where did we land on this PR? Do we want to merge it? Or do we want to close it and push users towards using caret and tilde notation instead? |
Bump. Any update on merging this? |
FWIW node seems to use the same "space padded hyphen" notation for ranges: https://docs.npmjs.com/misc/semver Also, the tests seem pretty comprehensive. |
Fixes #843
Closes #1232
This pull request implements @KristofferC's suggestion in this PR comment.
cc: @KristofferC @StefanKarpinski @00vareladavid @fredrikekre @goretkin @tkf